CI: https://ci.nodejs.org/job/node-test-pull-request/24964/

@cclauss I force-pushed your branch after I squashed the commits and rewrote the commit message to start with tools: .

Also, kicked off a build.

I support fast-tracking this if it is green in CI.

CI: https://ci.nodejs.org/job/node-test-pull-request/24965/

Also, kicked off a build.

I support fast-tracking this if it is green in CI.

No objections to fast tracking but as far as I can remember this file is only used to download ICU which the normal builds don't do (they use the ICU in deps).

./confugure.py imports this file and seems to use its functions in multiple places.

https://github.com/nodejs/node/search?q=nodedownload&unscoped_q=nodedownload

@nodejs/platform-windows Above CI failure seems irrelevant to this PR, which only changed some python syntax: https://ci.nodejs.org/job/node-test-binary-windows-2/2533/ It looks to be related to temp file cleanup, which I think has an open windows PR somewhere to fix it, but I haven't been able to find it.

./confugure.py imports this file and seems to use its functions in multiple places.

https://github.com/nodejs/node/search?q=nodedownload&unscoped_q=nodedownload

From memory, only on paths where ICU is downloaded (e.g. if deps/icu-small isn't there (it is in the default git clone/source tree) or --download is used with configure).

If its tested and passed, seems OK to fast-track.

If its not covered by the tests... then leaving it not-fasttracked won't make it better :-)

Will a ./configure --download=all be sufficient for a manual test, @richardlau ? I've never used icu, much less built variants of it.

If its tested and passed, seems OK to fast-track.

If its not covered by the tests... then leaving it not-fasttracked won't make it better :-)

😁

Will a ./configure --download=all be sufficient for a manual test, @richardlau ? I've never used icu, much less built variants of it.

./configure --download=all --with-intl=full-icu should work (omitting --with-intl=... defaults to small-icu which uses deps/icu-small if it exists).

With this PR and python2:

bash-4.2$ ./configure --download=all --with-intl=full-icu
 <https://github.com/unicode-org/icu/releases/download/release-64-2/icu4c-64_2-src.tgz>
 Fetch: . 23.5MB total, 23.5MB downloaded
Checking file integrity with md5:
md5:      a3d18213beec454e3cdec9a3116d6b05  deps/icu4c-64_2-src.tgz
 Extracting tarfile: deps/icu4c-64_2-src.tgz
INFO: Using floating patch "tools/icu/patches/64/source/common/putil.cpp" from "tools/icu"
INFO: Using floating patch "tools/icu/patches/64/source/i18n/dtptngen.cpp" from "tools/icu"
INFO: configure completed successfully
bash-4.2$

So we should be no worse off than we are currently.

OK, well fast-track or not, its all green in CI, 5 approvals, can be landed in 44 hours.

It looks to be related to temp file cleanup, which I think has an open windows PR somewhere to fix it

It landed. It's at #28858.

Windows tmpdir cleanup issues have been worked on by @joaocgreis and, before that, @refack.

I rebased this onto master, and kicked off another ci

CI: https://ci.nodejs.org/job/node-test-pull-request/24987/

None of the failures I see there seem to be related to this PR, and the following CI was green so this can land.

The failure in Windows 2008 shows a ENOTEMPTY for a directory that seems empty. I want to investigate things like this, so thanks for mentioning me.

The failures on other Windows versions show a problem with git fetching from the temp repo. Looking at Jenkins timestamps, the git push in the compile job completed before the test jobs started the git fetch, so this might be an issue in Jenkins or Git. We'll have to investigate this if it happens again.

Thanks for the analysis @joaocgreis -- and for the work, @cclauss

Landed in 8ae79c9